fix(slider): correct click/slide event position #1303
Conversation
googlebot
added
the
cla: yes
|
The slider's dimensions were only being updated when created/resized. If the sliders position changes outside of these conditions the calculations for the thumb etc. were out. It appears you only need one of the updates (either in Is there a better way to account for position changes outside of the elements control? |
| @@ -182,6 +182,7 @@ export class MdSlider implements AfterContentInit, ControlValueAccessor { | |||
|
|
|||
| this.isActive = true; | |||
| this.isSliding = false; | |||
| this._sliderDimensions = this._renderer.getSliderDimensions(); | |||
There was a problem hiding this comment.
If this is only necessary in one handler, I would just leave it in the click handler. Also needs a comment explaining why the dimensions need to be recalculated here.
There was a problem hiding this comment.
Hi @jelbourn, two things:
- to make the unit tests pass,
_sliderDimensionsneeds to be updated in bothonClickandonSlideStart, though it may be because the tests generate the click/slide events explicitly. - initially I couldn't write a failing test, I believe it is due to the way the click/slide events are generated in the tests.
The click/slide events in the unit tests take a "percentage", grab the up-to-date dimensions of the element and calculate the event coordinates based on that, which is backwards to how the browser does it. This means the values are always "correct" even if you move the sliders' parent around. See: https://github.com/angular/material2/blob/master/src/lib/slider/slider.spec.ts#L688
Adjusting these methods to take (pre-calculated) coordinates of an event, tests fail/pass as expected. Am I missing something obvious though...?
|
This partially fixes the issue, but if the slider has an initial value it still won't show up properly. #1663 refactors this component to use percentage-based measurements, rather than pixel based ones for the thumb position which should catch this last edge case. |
|
Superseded #1663 |
|
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
fixes #1234